Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add nodeRef alternative instead of internal findDOMNode #559

Merged
merged 7 commits into from
May 5, 2020
Merged

feat: add nodeRef alternative instead of internal findDOMNode #559

merged 7 commits into from
May 5, 2020

Conversation

iamandrewluca
Copy link
Contributor

@iamandrewluca iamandrewluca commented Oct 8, 2019

RFC: An alternative to ReactDOM.findDOMNode

  • Add nodeRef alternative for internal findDOMNode
  • Adapted tests
  • Added Tests
  • Adapted stories

We can name prop one of this:

  • domRef
  • nodeRef
  • guestRef
  • transitionRef
  • ref (with forwardRef but BREAKING CHANGE)

PRs:

Closes #457
Closes #486
Closes #514

Issues:

Closes #287
Closes #429

@iamandrewluca iamandrewluca changed the title refactor(Transition): use ref instead of findDOMNode refactor(Transition/ReplaceTransition): use ref instead of findDOMNode Oct 8, 2019
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this going to break stuff when rendering functional children that can't accept refs?

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Oct 8, 2019

@taion I'll add some tests with different types of components, and see whats happening.

I'm reading official docs. And here is saying that findDOMNode also does not support function components.

findDOMNode cannot be used on function components.
https://reactjs.org/docs/react-dom.html#finddomnode

They mean that cannot use it inside a function component.

@einarq
Copy link

einarq commented Nov 12, 2019

Any ideas on when (and if) this will be merged?

@einarq
Copy link

einarq commented Nov 12, 2019

@taion I'll add some tests with different types of components, and see whats happening.

I'm reading official docs. And here is saying that findDOMNode also does not support function components.

findDOMNode cannot be used on function components.
https://reactjs.org/docs/react-dom.html#finddomnode

They mean that cannot use it inside a function component.

The way I understand this PR, I would say it is going to break things. Whatever you want to transition would have to either accept a ref (a forwardRef component), or be a plain dom element.

Or am I missing something? It's happened before, many times... :)

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Nov 12, 2019

The way I understand this PR, I would say it is going to break things. Whatever you want to transition would have to either accept a ref (a forwardRef component), or be a plain dom element.

@einarq you are absolutely right. Whatever you want to transition would have to resolve to a DOM ref. So this would be a breaking change.

@taion what are your thoughts on creating a new major version, using ref instead of findDOMNode?

@danielhusar
Copy link
Contributor

danielhusar commented Nov 18, 2019

+1 for a new major version

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Nov 19, 2019

Or I'll try to find a solution that will
When user provides the ref use it to make transition (kind of breaking change also).
When user is not providing the ref, use findDOMNode and warn about it's usage, and removal in new major version, encourage to use the ref solution.

@inomdzhon
Copy link

inomdzhon commented Nov 21, 2019

Hi, there :)

I see that these changes lead to the fact that I need to wrap my component to React.forwardRef. I don't what it, because it additional payload.

What about provide this:

type TProps = {
  hostRef: React.Ref<HTMLElement>;
};
function Button(props: TProps) {
  return <button ref={props.hostRef}>...</button>
}

function App() {
  return (
    <CSSTransition refHook="hostRef">
       <button>...</button>
    </CSSTransition >
  );
}

If user provide refHook and component have refHook than use refHook, otherwise use ref.

I can create PR.

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Nov 21, 2019

@inomdzhon I also thought of this, user should pas to Transition a ref that will resolve to a DOM element.

function App() {
  const ref = React.useRef()
  
  return (
    <Transition domRef={ref}>
      <MaybeDOMmaybeFunctionMaybeClass refThatComponentAccepts={ref} />
    </Transition>
  )
}

Using the method above, we can make it non breaking change, and use findDOMNode when domRef is missing.

I am also thinking of writing a experimental hook. Maybe something like this

function App() {
  const ref = React.useRef()
  useTransition(ref)

  return (
    <MaybeDOMmaybeFunctionMaybeClass refThatComponentAccepts={ref} />
  )
}

@inomdzhon
Copy link

Hm, but what about list of component?

function App() {
  return (
    <TransitionGroup>
        <CSSTransition><div>1</div></CSSTransition>
        <CSSTransition><div>2</div></CSSTransition>
        <CSSTransition><div>3</div></CSSTransition>
        <CSSTransition><div>4</div></CSSTransition>
    </TransitionGroup>
  );
}

It can be hard to collect all refs and provide to CSSTransition. So, what about this case?

May be we can use render props (like Transition API);

<CSSTransition>
   {(state, hostRef) => <div className={state} ref={hostRef}></div>}
</CSSTransition>

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Nov 21, 2019

@inomdzhon in this case we can have this maybe?

function App() {
  return (
    <TransitionGroup>
      <CSSTransition domRef="hostRef"><div>1</div></CSSTransition>
      <CSSTransition domRef="hostRef"><div>2</div></CSSTransition>
      <CSSTransition domRef="hostRef"><div>3</div></CSSTransition>
    </TransitionGroup>
  );
}

or even better this

function App() {
  return (
    <TransitionGroup domRef="hostRef">
      <CSSTransition><div>1</div></CSSTransition>
      <CSSTransition><div>2</div></CSSTransition>
      <CSSTransition><div>3</div></CSSTransition>
    </TransitionGroup>
  );
}

And domRef can be a string or a ref. In case of TransitionGroup only string

@inomdzhon
Copy link

I think <CSSTransition refHook="hostRef"> will be more clear than provide a responsibility to TransitionGroup.

For don't repeat users can customize CSSTransition prop like this

function CSSTransitionAdapter(props) {
  return <CSSTransition refHook="hostRef" {...props} />
}

Than

function App() {
  return (
    <TransitionGroup>
      <CSSTransitionAdapter><div>1</div></CSSTransition>
      <CSSTransitionAdapter><div>2</div></CSSTransition>
      <CSSTransitionAdapter><div>3</div></CSSTransition>
    </TransitionGroup>
  );
}

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Nov 21, 2019

This are all use cases I can see for the API right now.

import React from 'react'
import { Transition, TransitionGroup } from 'react-transition-group'

function MaybeDomOrFunctionOrClass(props) {
  const { refThatResolvesToDOM } = props
  return <div ref={refThatResolvesToDOM}>666</div>
}

function App() {
  const ref = React.useRef(null)
  const ref1 = React.useRef(null)
  const ref2 = React.useRef(null)
  const ref3 = React.useRef(null)

  return (
    <React.Fragment>

      /* it will use ref to make transition */
      <Transition domRef={ref}>
        <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref} />
      </Transition>

      /* it will try to find and use prop `refThatResolvesToDOM` */
      /* otherwise fallback to `findDOMNode` */
      <Transition domRef="refThatResolvesToDOM">
        <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref} />
      </Transition>

      /* it will try to find and use prop `refThatResolvesToDOM` */
      /* otherwise fallback to `findDOMNode` */
      /* ??? maybe create a ref internally in Transition and pass as `refThatResolvesToDOM` to child */
      <Transition domRef="refThatResolvesToDOM">
        <MaybeDomOrFunctionOrClass />
      </Transition>

      <TransitionGroup>
        <Transition domRef={ref1}>
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref1} />
        </Transition>
        <Transition domRef={ref2}>
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref2} />
        </Transition>
        <Transition domRef={ref3}>
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref3} />
        </Transition>
      </TransitionGroup>

      <TransitionGroup>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref1} />
        </Transition>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref2} />
        </Transition>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass refThatResolvesToDOM={ref3} />
        </Transition>
      </TransitionGroup>

      <TransitionGroup>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass />
        </Transition>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass />
        </Transition>
        <Transition domRef="refThatResolvesToDOM">
          <MaybeDomOrFunctionOrClass />
        </Transition>
      </TransitionGroup>

      <TransitionGroup domRef="refThatResolvesToDOM">
        <Transition>
          <MaybeDomOrFunctionOrClass />
        </Transition>
        <Transition>
          <MaybeDomOrFunctionOrClass />
        </Transition>
        <Transition>
          <MaybeDomOrFunctionOrClass />
        </Transition>
      </TransitionGroup>

    </React.Fragment>
  )
}

@inomdzhon
Copy link

inomdzhon commented Nov 22, 2019

Ok, I see.

What about use useRef for hook version of library (like useTransition, useCSSTransition) and for basic version (like <CSSTransition />) use React.cloneElement({ [props.refHook || 'ref']: handleRef });?

The fewer use cases, the easier it is to use and maintain code. Over the years, I realized that the principle the easier the better. I mean we shouldn't complicate.

@jRichardeau
Copy link

Hey,
Do you know when this will be released ?
Thanks

@camflan
Copy link

camflan commented Feb 25, 2020

Any updates on release schedule?

@inomdzhon
Copy link

Hi, @iamandrewluca 👋

How I can help you?

@iamandrewluca iamandrewluca changed the title refactor(Transition/ReplaceTransition): use ref instead of findDOMNode wip: refactor(Transition/ReplaceTransition): use ref instead of findDOMNode Feb 26, 2020
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Feb 27, 2020

Still thinking how to provide the API for the end user.
Because it seems this change will propagate to top most end user.
For example https://github.com/reactstrap/reactstrap is using Transition in Fade component, that is used by Alert component, that is used by the end user.
Here reactstrap or creates the ref internally, or let the user to pass the prop.
Which is not so convinient.

The limitation of creating the ref internally is that we don't know what kind of children the Transition has, Class, Function, DOM.
To make this possible, react-transition-group need to make a breaking change, and force children to have a ref that resolves to a DOM element. But this will force a breaking change chain for all tree dependency, that will want to upgrade.

@iamandrewluca
Copy link
Contributor Author

If taking the example above from #559 (comment)

And changing MaybeDomOrFunctionOrClass from

function MaybeDomOrFunctionOrClass(props) {
  const { refThatResolvesToDOM } = props
  return <div ref={refThatResolvesToDOM}>666</div>
}

to

function MaybeDomOrFunctionOrClass() {
  const ref = React.useRef(null)
  return (
    <Transition domRef={ref}>
      <div ref={ref}>666</div>
    </Transition>
  )
}

then our App resolves to

function App() {
  return (
    <React.Fragment>
      
      <MaybeDomOrFunctionOrClass />

      <TransitionGroup>
        <MaybeDomOrFunctionOrClass />
        <MaybeDomOrFunctionOrClass />
        <MaybeDomOrFunctionOrClass />
      </TransitionGroup>

    </React.Fragment>
  )
}

@iamandrewluca
Copy link
Contributor Author

If MaybeDomOrFunctionOrClass wants to expose for example a innerRef props

function MaybeDomOrFunctionOrClass({ innerRef = React.useRef(null) }) {
  return (
    <Transition domRef={innerRef}>
      <div ref={innerRef}>666</div>
    </Transition>
  )
}
function App() {
  const ref = React.useRef(null)
  const ref1 = React.useRef(null)
  const ref2 = React.useRef(null)
  const ref3 = React.useRef(null)
  return (
    <React.Fragment>
      
      <MaybeDomOrFunctionOrClass innerRef={ref} />

      <TransitionGroup>
        <MaybeDomOrFunctionOrClass innerRef={ref1} />
        <MaybeDomOrFunctionOrClass innerRef={ref2} />
        <MaybeDomOrFunctionOrClass innerRef={ref3} />
      </TransitionGroup>

    </React.Fragment>
  )
}

@iamandrewluca iamandrewluca changed the title wip: refactor(Transition/ReplaceTransition): use ref instead of findDOMNode wip: feat(Transition): add domRef alternative instead of findDOMNode Feb 27, 2020
@iamandrewluca
Copy link
Contributor Author

I'll do some tests using this PR and reactstrap components.

@inomdzhon
Copy link

Yeah, I understood 👌Waiting tests.

Furthermore, I realized on this point refHook is string – we can't check type children, we can only do a run-time check.

@iamandrewluca iamandrewluca changed the title wip: feat(Transition): add domRef alternative instead of findDOMNode wip: feat(Transition/ReplaceTransition): add domRef alternative instead of findDOMNode Mar 2, 2020
@iamandrewluca
Copy link
Contributor Author

celebrate

@silvenon silvenon merged commit 85016bf into reactjs:master May 5, 2020
jquense pushed a commit that referenced this pull request May 5, 2020
# [4.4.0](v4.3.0...v4.4.0) (2020-05-05)

### Features

* add `nodeRef` alternative instead of internal `findDOMNode` ([#559](#559)) ([85016bf](85016bf))
@jquense
Copy link
Collaborator

jquense commented May 5, 2020

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iamandrewluca iamandrewluca deleted the remove-find-dom-node branch May 5, 2020 18:56
@silvenon
Copy link
Collaborator

silvenon commented May 5, 2020

I didn't realize we have semantic-release set up 😅 I'll update release notes and the documentation.

@fkhadra
Copy link

fkhadra commented May 5, 2020

Guys, thank you so much for all the work you did for addressing this issue. Also, migrating my library to the new api was a breeze. 🙏

@silvenon
Copy link
Collaborator

silvenon commented May 5, 2020

I'm glad that you're already enjoying the update! ❤️ Let us know if there are any issues.

@malikalimoekhamedov
Copy link

malikalimoekhamedov commented May 20, 2020

I feel like the typing of nodeRef should be extended. Right now it only accommodates for the type 'Element' which is limiting in case of nested routes. Please, let me know if I misunderstood #559.

Warning: Failed prop type: Invalid prop `nodeRef.current` of type `Switch` supplied to `CSSTransition`, expected instance of `Element`.

Here's my situation:

Level 1 router is a <Switch> handling switching between high-level website sections which, themselves are routers handling level 2 navigation (<Switch>'es).
This means that I'd like to transition <Switch> components on level 1 instead of regular div's or the likes.

Right now, I'm drilling nodeRef down to div's through nested <Switch>'es and it: a) feels hacky, b) interferes with level 2 CSSTransitions.

Am I missing something? Is the only way around this to wrap all of my <Switch>'es in <div ref={nodeRef}>'s?

@iamandrewluca
Copy link
Contributor Author

@antikvarBE can you provide a simple example? I kind of understand what are you trying to do, and I think I know what should you do, but I don't see entire picture.

@malikalimoekhamedov
Copy link

malikalimoekhamedov commented May 20, 2020

Thanks for reaching back @iamandrewluca .
Here's what I thought was allowed:

In Private.tsx

  const nodeRef = React.useRef(null);

  return (
    <div id={'private'} className={styles.wrapper}>
      <MainMenu items={items} />
      <main className={styles.content}>
        <SwitchTransition mode={'out-in'}>
          <CSSTransition
            key={location.pathname.split('/', 3).join('/')}
            timeout={500}
            classNames={{ ...styles }}
            nodeRef={nodeRef}
          >
              <PrivateRouter location={location} ref={nodeRef} />
          </CSSTransition>
        </SwitchTransition>
      </main>
    </div>
  );

In PrivateRouter.routes.tsx

const PrivateRouter: React.FC<SwitchProps & {nodeRef: React.MutableRefObject<null>}> = ({ location }) => (
  <Switch location={location} ref={nodeRef}>
    <Route path={'/favorites'}>
      <Favorites />
    </Route>
    <Route path={'/support'}>
      <Support />
    </Route>
    <Route path={'/account'}>
      <Account />
    </Route>
  </Switch>
);

Doing this would throw the aforementioned error:

Warning: Failed prop type: Invalid prop `nodeRef.current` of type `Switch` supplied to `CSSTransition`, expected instance of `Element`.

So, currently, I'm doing this instead:

const nodeRef = React.useRef(null);

  return (
    <div id={'private'} className={styles.wrapper}>
      <MainMenu items={items} />
      <main className={styles.content}>
        <SwitchTransition mode={'out-in'}>
          <CSSTransition
            key={location.pathname.split('/', 3).join('/')}
            timeout={500}
            classNames={{ ...styles }}
            nodeRef={nodeRef}
          >
            <div ref={nodeRef}> // <-- This is my current workaround
              <PrivateRouter location={location} />
            </div>
          </CSSTransition>
        </SwitchTransition>
      </main>
    </div>
  );

It works perfectly fine but I'm the kind of person who thinks twice before introducing another DOM level and if I can get away without doing it and keep things nice and clean semantically, I'd rather do that.

What do you think?

@silvenon
Copy link
Collaborator

Is Switch coming from react-router? What does its ref prop usually return? I couldn't find it in the docs. Unless it returns a DOM Node, it shouldn't be used with nodeRef.

@malikalimoekhamedov
Copy link

@silvenon , it's from react-router-dom.
A quick type inspection of Switch's ref yields the following:

(JSX attribute) React.ClassAttributes<Switch>.ref?: string | ((instance: Switch | null) => void) | React.RefObject<Switch> | null | undefined

@silvenon
Copy link
Collaborator

silvenon commented May 20, 2020

Oh, then ref points to a class instance, not a DOM element, therefore you shouldn't use it directly (that's why it's named nodeRef). If Switch happens to expose some kind of a instance property that points to a DOM node, then use that (probably not advisable as it would have to use findDOMNode for that), otherwise your workaround is the best way to proceed, IMO.

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented May 20, 2020

@antikvarBE in example above if I'm not mistaken you want to transition between private routes? What version of react router do you use?
@silvenon I'll try to add an example in stories to reflect this use case.

@malikalimoekhamedov
Copy link

malikalimoekhamedov commented May 20, 2020

@iamandrewluca , I'm using "react-router": "^5.2.0" and "react-router-dom": "^5.2.0". Yes, I'm transitioning between private routes as they're described in Private.routes.tsx (see example). Essencially, <Switch> displays one and only one <Route> at a time. If one route only contains one component, it also means <Switch> displays one and only one component at a time. That's the way I understand it.

@malikalimoekhamedov
Copy link

malikalimoekhamedov commented May 20, 2020

Oh, then ref points to a class instance, not a DOM element, therefore you shouldn't use it directly (that's why it's named nodeRef). If Switch happens to expose some kind of a instance property that points to a DOM node, then use that (probably not advisable as it would have to use findDOMNode for that), otherwise your workaround is the best way to proceed, IMO.

@silvenon : Got it. That makes sense. Just wanted to double-check. Thanks a bunch.

johnfrench3 pushed a commit to johnfrench3/transition-group-react that referenced this pull request Nov 2, 2022
# [4.4.0](reactjs/react-transition-group@v4.3.0...v4.4.0) (2020-05-05)

### Features

* add `nodeRef` alternative instead of internal `findDOMNode` ([#559](reactjs/react-transition-group#559)) ([85016bf](reactjs/react-transition-group@85016bf))
patrickm68 added a commit to patrickm68/react-transition-group-developer that referenced this pull request Dec 1, 2022
# [4.4.0](reactjs/react-transition-group@v4.3.0...v4.4.0) (2020-05-05)

### Features

* add `nodeRef` alternative instead of internal `findDOMNode` ([#559](reactjs/react-transition-group#559)) ([85016bf](reactjs/react-transition-group@85016bf))
shaikdev2 pushed a commit to shaikdev2/transition-group-react that referenced this pull request Jun 9, 2023
# [4.4.0](reactjs/react-transition-group@v4.3.0...v4.4.0) (2020-05-05)

### Features

* add `nodeRef` alternative instead of internal `findDOMNode` ([#559](reactjs/react-transition-group#559)) ([85016bf](reactjs/react-transition-group@85016bf))
GreenCat1996 added a commit to GreenCat1996/react-transition-group that referenced this pull request Aug 1, 2023
# [4.4.0](reactjs/react-transition-group@v4.3.0...v4.4.0) (2020-05-05)

### Features

* add `nodeRef` alternative instead of internal `findDOMNode` ([#559](reactjs/react-transition-group#559)) ([85016bf](reactjs/react-transition-group@85016bf))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet